Skip to content

feat: Support putting TrustStore information in Secret #597

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: Support putting TrustStore information in Secret #597

wants to merge 11 commits into from
+169 −68

Conversation

sbernauer
Copy link
Member

@sbernauer sbernauer commented May 8, 2025 •
edited
Loading

Description

Small follow-up of #557

This PR allows the Trustore information (such as the ca.crt) to not only be put in a ConfigMap, but also a Secret.
This was requested by a customer, they mentioned it is required by OpenShift routes, but didn't check that.

The first commit is only test refactoring, I'd suggest reviewing it separately.

CRD change

apiVersion: secrets.stackable.tech/v1alpha1
kind: TrustStore
spec:
  secretClassName: tls
  format: tls-pem

  # New field targetKind:
  targetKind: ConfigMap # or Secret, defaults to ConfigMap

Actual schema change

                targetKind:
                  default: ConfigMap
                  description: |-
                    Which Kubernetes resource should be used to output the requested information to.
                    The trust information (such as a `ca.crt`) can be considered public information, so we put it in a `ConfigMap` by default. However, some tools (such as OpenShift routes) require it to be placed in a `Secret`, so we also support that.
                    Can be either `ConfigMap` or `Secret`, defaults to `ConfigMap`.
                  enum:
                    - Secret
                    - ConfigMap
                  type: string

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

  • Changes are OpenShift compatible
  • CRD changes approved
  • CRD documentation for all fields, following the style guide.
  • Helm chart can be installed and deployed operator works
  • Integration tests passed (for non trivial changes)
  • Changes need to be "offline" compatible

Reviewer

  • Code contains useful comments
  • Code contains useful logging statements
  • (Integration-)Test cases added
  • Documentation added or updated. Follows the style guide.
  • Changelog updated
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

  • Feature Tracker has been updated
  • Proper release label has been added
  • Roadmap has been updated

@nightkr
Copy link
Contributor

nightkr commented May 8, 2025

Hm, Route.spec.tls.destinationCACertificate looks like it wants the certificate inline, so I'm not sure how this would help.

I also have a vague memory of that we ran into some issues around OpenShift expecting the certificates to use relative hostnames rather than FQDNs...

@sbernauer
Copy link
Member Author

I had the same observation, but I think I wouldn't focus to much an OpenShift in particular here.
Customer didn't reply yet, but I guess it's not unrealistic that some tools requires it as Secret, so given the easy implementation I personally would just add it. Would really suck for a customer if he can't use TrustStore because an oversight.

@sbernauer sbernauer self-assigned this May 19, 2025
@sbernauer sbernauer mentioned this pull request May 9, 2025
Closed
@lfrancke lfrancke moved this from Next to Selected for Development in Stackable End-to-End Coordination Jun 27, 2025
@lfrancke lfrancke moved this from Selected for Development to In Refinement in Stackable End-to-End Coordination Jul 31, 2025
@sbernauer sbernauer force-pushed the feat/truststore-as-secret branch from a78d7f7 to 2dbc207 Compare August 1, 2025 12:21
@lfrancke lfrancke moved this from In Refinement to In Progress in Stackable End-to-End Coordination Aug 6, 2025
@siegfriedweber
Copy link
Member

siegfriedweber commented Aug 6, 2025 •
edited
Loading

The field outputResource could be named targetResource (similar to targetPort in Services) or targetKind because Secret and ConfigMap are kinds, not resources (which are instances of kinds).

@sbernauer
Copy link
Member Author

I searched a bit and could not find a real world CRD example. But happy if someone else finds one :)
@siegfriedweber I like the idea of targetKind, will update the PR

@sbernauer
Copy link
Member Author

Please vote for this CRD change on this comment

@sbernauer
Copy link
Member Author

Hm, Route.spec.tls.destinationCACertificate looks like it wants the certificate inline, so I'm not sure how this would help.

A customer today asked for this, and mentioned

Could we tell the TrustStore CRD to create a secret instead of a ConfigMap?
The route config on an Ingress only accepts secret references, not ConfigMaps:
https://docs.redhat.com/en/documentation/openshift_container_platform/4.14/html/networking/configuring-routes#creating-re-encrypt-route-with-custom-certificate_route-configuration

this should clarify that OpenShift expects it as a Secret

@sbernauer
Copy link
Member Author

Decision approved

@sbernauer sbernauer moved this to Development: In Progress in Stackable Engineering Aug 11, 2025
@sbernauer sbernauer moved this from Development: In Progress to Development: Waiting for Review in Stackable Engineering Aug 12, 2025
@sbernauer sbernauer requested review from a team and removed request for nightkr August 12, 2025 08:47
NickLarsenNZ
NickLarsenNZ previously approved these changes Aug 21, 2025
View reviewed changes
Copy link
Member

@NickLarsenNZ NickLarsenNZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do we need doc updates?

deploy/helm/secret-operator/crds/crds.yaml
@@ -395,6 +395,18 @@ spec:
secretClassName:
description: The name of the SecretClass that the request concerns.
type: string
targetKind:
default: ConfigMap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed the boat on this, but I think Secret should be the default.
I assume ConfigMap was chosen as a default for good reason, so please continue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess because that was the existing behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, existing behavior is a ConfigMap. If we would default to Secret it's breaking ;)
Also I recall someone in a discussion saying that we should try to be transparent that this is not confidential data.

@NickLarsenNZ NickLarsenNZ moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Aug 21, 2025
@sbernauer
Copy link
Member Author

Do we need doc updates?

Added to concepts page in 7613599

@sbernauer sbernauer requested a review from NickLarsenNZ August 21, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siegfriedweber siegfriedweber Awaiting requested review from siegfriedweber

@NickLarsenNZ NickLarsenNZ Awaiting requested review from NickLarsenNZ

At least 1 approving review is required to merge this pull request.

Assignees

@sbernauer sbernauer

Labels
customer-request size/S
Projects
Stackable End-to-End Coordination
Status: In Progress
Stackable Engineering
Status: Development: In Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sbernauer @nightkr @siegfriedweber @NickLarsenNZ @lfrancke